[VectorCombine] Preserve scoped alias metadata #153714
Conversation
|
@llvm/pr-subscribers-vectorizers @llvm/pr-subscribers-llvm-transforms Author: Kyle Wang (knwng) ChangesRight now if a load op is scalarized, the Full diff: https://github.com/llvm/llvm-project/pull/153714.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/VectorCombine.cpp b/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
index 4a681cbdab8ca..587889873a778 100644
--- a/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
+++ b/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
@@ -1811,6 +1811,10 @@ bool VectorCombine::scalarizeLoadExtract(Instruction &I) {
// erased in the correct order.
Worklist.push(LI);
+ LLVMContext &ctx = LI->getContext();
+ unsigned aliasScopeKind = ctx.getMDKindID("alias.scope");
+ unsigned noAliasKind = ctx.getMDKindID("noalias");
+
// Replace extracts with narrow scalar loads.
for (User *U : LI->users()) {
auto *EI = cast<ExtractElementInst>(U);
@@ -1831,6 +1835,14 @@ bool VectorCombine::scalarizeLoadExtract(Instruction &I) {
LI->getAlign(), VecTy->getElementType(), Idx, *DL);
NewLoad->setAlignment(ScalarOpAlignment);
+ if (MDNode *aliasScope = LI->getMetadata(aliasScopeKind)) {
+ NewLoad->setMetadata(aliasScopeKind, aliasScope);
+ }
+
+ if (MDNode *noAlias = LI->getMetadata(noAliasKind)) {
+ NewLoad->setMetadata(noAliasKind, noAlias);
+ }
+
replaceValue(*EI, *NewLoad);
}
|
|
Hi @knwng, thanks! I haven't look at the patch in any detail. It does need a lit test though. |
nikic
changed the title
|
|
||
| if (MDNode *noAlias = LI->getMetadata(noAliasKind)) { | ||
| NewLoad->setMetadata(noAliasKind, noAlias); | ||
| } |
There was a problem hiding this comment.
If possible, it would be better to use getAAMetadata() and adjustForAccess() here.
There was a problem hiding this comment.
Hi @nikic, thank you for the advice. I know it's a more thoughtful solution. But I'm not familiar with AA in LLVM. Can you guide me on how to use adjustForAccess(), like how to correctly calculate offset and type? Thanks!
There was a problem hiding this comment.
The type is VecTy->getElementType(). The offset would be Idx * the type size like here:
llvm-project/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
Lines 1652 to 1654 in adce747
The problem is if Idx is a variable. In that case the offset is unknown. So I would make the whole logic conditional on the dyn_cast<ConstantInt>(Idx) case.
RKSimon
left a comment
RKSimon
left a comment
There was a problem hiding this comment.
Did you find this by inspection or was there a real world case that we can create test from please?
It's found in a real world case. @frederik-h 's test is extracted from it. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| cl::desc("Disable all vector combine transforms")); | ||
| static cl::opt<bool> | ||
| DisableVectorCombine("disable-vector-combine", cl::init(false), cl::Hidden, | ||
| cl::desc("Disable all vector combine transforms")); |
There was a problem hiding this comment.
Please do not reformat unrelated code (you can use git-clang-format to only format changed lines).
|
Hi @nikic, is the CI failure a runner issue? |
Looks like it is. If you retrigger the run, e.g. after you resolve the conflict, it will probably work. |
Head branch was pushed to by a user without write access
6 participants
Right now if a load op is scalarized, the
!alias.scopeand!noaliasmetadata are dropped. This PR is to keep them if exist.